Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker build tip #2692

Closed
wants to merge 2 commits into from
Closed

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 3, 2017

I found a solution for my issue regarding local permissions between my host machine and the docker container and I added a Tip inside the docker section.

Fixes one of the issues reported at #2658

@humitos humitos added PR: work in progress Pull request is not ready for full review PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Mar 3, 2017
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 on this change, it seems it would confuse users. I think there is a proper fix to this that doesn't involve building a new image. We should find out what that is and make sure that is documented instead.

@humitos
Copy link
Member Author

humitos commented Mar 4, 2017

I think there is a proper fix to this that doesn't involve building a new image. We should find out what that is and make sure that is documented instead.

I agree with that, but I didn't find another (better) way yet. On the other hand, the RTD image could have more pre-defined users with a couple of gid and uid (1000-1005, for example, since they are really common). I don't know.

@agjohnson agjohnson added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels Mar 9, 2017
@humitos
Copy link
Member Author

humitos commented May 16, 2017

2 different ideas to fix the docker/developer workflow:

  1. use a LOCAL_USER_ID = "os.system('tid -u')" setting with the current user as default -probably with a better way of get it. For this, we will need to install conda for all the users instead of only docs. So, probably for this we could mount envs directories like how we mount the user_builds
  2. another idea that could allow first-time (or 1-time) contributors is to have a docker image for the whole RTD Django Project and follow these steps:
    1. clone the repo
    2. run docker run <image>
    3. modify the code as you wish
    4. run docker run <image>
    5. make the PR

cc @agjohnson

@humitos
Copy link
Member Author

humitos commented May 18, 2017

I'm just adding this here to keep it together in the same place since I had to modify my old hack to allow conda. So, this is my new hacky Dockerfile image:

# Read the Docs - Environment base
FROM readthedocs/build:2.0
MAINTAINER Manuel Kaufmann <humitos@gmail.com>
LABEL version="humitos"

USER root

# UID and GID from readthedocs/user
RUN groupadd --gid 1000 humitos
RUN useradd -m --uid 1000 --gid 1000 humitos

USER humitos

# Install miniconda as humitos user
WORKDIR /home/humitos
RUN curl -O https://repo.continuum.io/miniconda/Miniconda2-4.3.11-Linux-x86_64.sh
RUN bash Miniconda2-4.3.11-Linux-x86_64.sh -b -p /home/humitos/miniconda2/

# we can't just use ENV and prepend our own conda since 'python2.7'
# will be taken from there and `virtualenv` is not installed. So, it fails.
# http://stackoverflow.com/questions/11650840/linux-remove-path-from-path-variable
RUN export PATH=`echo $PATH | sed -e 's/:\/home\/docs\/miniconda2\/bin\/$//'`
ENV PATH=$PATH:/home/humitos/miniconda2/bin

# Add conda-forge channel with the highest channel priority
RUN conda config --add channels conda-forge


CMD ["/bin/bash"]

@humitos
Copy link
Member Author

humitos commented Dec 21, 2017

We still don't have a better solution for this and it's the setup that I'm using to develop but I think, as Anthony said, this shouldn't be like this (the default option to develop), but on the other hand, we don't have anything else.

Should I close this PR?

@agjohnson is your setup with a better choice to suggest to newcomers?

@ericholscher what's your setup? is it shareable or it's more complex?

@ericholscher
Copy link
Member

I just.. run docker? I don't have anything special I don't think about the users in my setup.

@humitos
Copy link
Member Author

humitos commented Dec 21, 2017

Mmm... This is weird then. Why it does just work for you but not for me or other people?

I'd like to have more information / talk about your setup or something because "just run docker" doesn't work on my Ubuntu nor Archlinux and I think it didn't work for Anthony also running OSX and I think that's why he uses Vagrant.

😕

@eine
Copy link

eine commented Dec 24, 2017

@humitos, in my case, it works well on Windows 10 using MinGW64 (MSYS2). See #48. Indeed, it is a VM, so I suppose it's the same case as Anthony's Vargrant. However, it does not work in a Travis job (which is Ubuntu Trusty): https://travis-ci.org/1138-4EB/ghdl/jobs/320951163

Just out of curiosity, why are you adding a docs user?


BTW, I fixed it by ensuring that each user writes only to directories which are already owned, and accesses any other directory with read-only permissions. I.e., I used docker cp:

    docker run --name sphinx -tv /$(pwd):/src readthedocs/build:latest bash -c "\
      pip3 install sphinx recommonmark sphinx_rtd_theme && \
      cp -r /src/doc ~/ && cd ~/doc && \
      make html"

    docker cp sphinx:/home/docs/doc/build/html doc/
    docker rm -f sphinx

@humitos
Copy link
Member Author

humitos commented Dec 24, 2017

hi @1138-4eb

Just out of curiosity, why are you adding a docs user?

I didn't create the docs user, it comes with the original RTD docker image. I created my own user inside the docker image (humitos) and I need to do this because of permission issues (explained in the issue linked in the description).

This docker image is used by the readthedocs.org code itself, it's not used as you mentioned the in the issue linked. I can run the original image as you did without any problem but when it's ran by RTD code itself there are a couple of problems that this Docker Build Tip helps to solve.

@agjohnson
Copy link
Contributor

@humitos After having some issues with virtualbox, I decided to give Docker for MacOS a try again. It seems to work pretty well now, so I've be using that. Like @ericholscher said, this solution just works, I don't worry about permissions or anything, it just runs.

@1138-4eb we need the docs user because we execute commands as a non-privileged user on both the container host and guest

I think as we discussed in another PR, a Dockerfile in contrib/ that has a configurable UID through a docker build arg is the closest we'll get.

@humitos
Copy link
Member Author

humitos commented Dec 24, 2017

I think as we discussed in another PR, a Dockerfile in contrib/ that has a configurable UID through a docker build arg is the closest we'll get.

If we all agree on this, I can create a PR with this and update the docs. Besides, it seems that this will be only necessary in Linux so I add a note about this.

@eine
Copy link

eine commented Dec 24, 2017

This docker image is used by the readthedocs.org code itself, it's not used as you mentioned the in the issue linked.

Where can I find the readthedocs code that uses the image with a given project? I.e. clone a repo with a .readthedocs.yml and run rtd build all. I saw how to spin RTD as a service and access it through the browser, but I'd like to reproduce a build without all that overhead.

I can run the original image as you did without any problem but when it's ran by RTD code itself there are a couple of problems that this Docker Build Tip helps to solve.

Where can I find info about those problems? Can docker cp be used in the scripts so that using the same user is not a requirement? I mean, even though the solutions you are proposing are technically correct, it seems ackward. It makes sense "only" if readthedocs needs to replace a single native command with a single docker run.

@1138-4eb we need the docs user because we execute commands as a non-privileged user on both the container host and guest

As explained above, you should be able to execute commands as two non-privileged users, one inside the container and a different one outside. It is 'cleaner' if each of them can only modify it's own sources, because it avoids the container modifying external resources which should not be touched, or generating auxiliary files that should not be left in the base repo.

@ericholscher
Copy link
Member

@humitos thoughts on this? I'm considering closing it, since it's quite old, but if it should be merged, we should do something with it :)

agjohnson added a commit that referenced this pull request Sep 5, 2018
This is necessary as permissions are all incorrect on the paths that are
shared between the host system and the Docker build container.

Closes #2692
@agjohnson
Copy link
Contributor

I automated this in #4608, we can probably proceed there

@ericholscher
Copy link
Member

Closing this in favor of #4608

humitos pushed a commit that referenced this pull request May 17, 2019
This is necessary as permissions are all incorrect on the paths that are
shared between the host system and the Docker build container.

Closes #2692
agjohnson added a commit that referenced this pull request Jun 13, 2019
* Add a contrib Dockerfile for local build image on Linux

This is necessary as permissions are all incorrect on the paths that are
shared between the host system and the Docker build container.

Closes #2692

* Use always latest image to build the Docker image

* Make image label configurable in docker build script

This re-reverts back to using label for some of the dockerfile. Also,
instead of creating a group, we simply groupmod the existing docs group.

This doesn't address the configuration changes necessary for development
images yet.

* Update command call for docker_build.sh

* Add configuration option that patches the docker image names in development

This setting can be used from local_settings.py, and signifies that you
have run ``docker_build.sh`` to build local development images that
replace the UID/GID.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants